Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't force reload tab when restoring state #2016

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Don't force reload tab when restoring state #2016

merged 3 commits into from
Jan 4, 2024

Conversation

ayoy
Copy link
Collaborator

@ayoy ayoy commented Jan 4, 2024

Task/Issue URL: https://app.asana.com/0/1177771139624306/1206278424786335/f

Description:
Tab is loaded on state restoration so we don't need to force reload it.

Steps to test this PR:

  1. Follow steps to reproduce in the linked Asana task and verify that it works as expected.
  2. Verify that auto-closing Zoom tabs still works:
    1. Make browser default
    2. Open a conversation with yourself in MM and start a Zoom call
    3. Allow the browser to open Zoom
    4. Verify that the tab was closed.
Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

@ayoy ayoy requested a review from Bunn January 4, 2024 09:51
@ayoy
Copy link
Collaborator Author

ayoy commented Jan 4, 2024

@Bunn I'm not entirely sure about the failing test:

  • The scenario (launching the app with an unclosed external scheme tab) shouldn't be happening after users are onboarded to the new app version.
  • If it even is an issue, I'd be tempted to let it in because what we're fixing here is far more severe.

I'll have another look at the failing test once I'm back, but if I can't figure out a fix I'd just skip it until Alex is back.

@Bunn
Copy link
Collaborator

Bunn commented Jan 4, 2024

@Bunn I'm not entirely sure about the failing test:

  • The scenario (launching the app with an unclosed external scheme tab) shouldn't be happening after users are onboarded to the new app version.
  • If it even is an issue, I'd be tempted to let it in because what we're fixing here is far more severe.

I'll have another look at the failing test once I'm back, but if I can't figure out a fix I'd just skip it until Alex is back.

I tried to reproduce manually the issue that the tests are failing and it seems to be working as expected, but I only timeboxed about 10min to look at it. Other than that it LGTM, so I'll approve and let you decide the best way forward. Please ping me if you need me to spend more time in this.

Bunn
Bunn approved these changes Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@Bunn Bunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was able to reproduce the issue before and after the fix seems to be working as expected. LGTM

@ayoy
Copy link
Collaborator Author

ayoy commented Jan 4, 2024

Thanks so much @Bunn!

I spent some time investigating it further and it seems to me that we need to distinguish regular reload from the reload caused by state restoration. So I split URLSource.stateRestoration into .pendingStateRestoration and .loadedByStateRestoration and updated the code so that forceReload() called on TabContent in .pendingStateRestoration marks it as .loadedByStateRestoration, and only subsequent reloads update source to .reload. This fixes the failing unit test, while preserving the fix for the bug and keeping tab autoclosing working.

Would you be able to have another look, please?

@ayoy ayoy requested a review from Bunn January 4, 2024 11:26
Bunn
Bunn approved these changes Jan 4, 2024
View reviewed changes
Copy link
Collaborator

@Bunn Bunn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes, LGTM

@ayoy ayoy merged commit 82dbdd6 into release/1.69.0 Jan 4, 2024
14 checks passed
@ayoy ayoy deleted the dominik/fix-tab-refreshing branch January 4, 2024 12:34
@tomasstrba
Copy link
Contributor

Can we cherry pick to the main too? It's slightly interfering with my tab previews logic

@ayoy
Copy link
Collaborator Author

ayoy commented Jan 4, 2024

I'm going to merge it into main once I make a release (in less than 30 minutes I hope :))

samsymons added a commit that referenced this pull request Jan 8, 2024
@samsymons
Merge branch 'main' into sam/netp-system-extension-crash-reports
145ee1e 
* main: (35 commits)
  Update login item failure pixel (#2024)
  fix turn off sync error message (#2025)
  DBP: macOS - Scheduler Progress Notifications (#2023)
  DBP: Implement sign-out flow for DBP (#2009)
  Switch CI to Xcode 15.1 (#2022)
  fix(duckplayer): bump CSS for duckplayer nav loop fix (#1982)
  Bump Submodules/privacy-reference-tests from `a3acc21` to `6b7ad1e` (#2006)
  Bump version to 1.69.0 (99)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  Don't force reload tab when restoring state (#2016)
  Bump version to 1.69.0 (98)
  Update embedded files
  Update NetP launch agent logic to include build number (#2015)
  Always use 'sandbox' Application Support directory for Favicons Fetcher (#2013)
  Allow calculations in the address bar (#2012)
  Fix clickable area for buttons in 'Sync with Another Device' view (#2011)
  Fix for empty autofill state displayed on top of existing password items (#1998)
  Update error messages (#1999)
  Autofill never save for site (#1991)
  ...
samsymons added a commit that referenced this pull request Jan 9, 2024
@samsymons
Merge branch 'main' into sam/data-broker-remote-messaging
6646d37 
# By Dax the Duck (6) and others
# Via Fernando Bunn (2) and others
* main: (36 commits)
  Send VPN system extension crashes to Sentry (#2002)
  Update login item failure pixel (#2024)
  fix turn off sync error message (#2025)
  DBP: macOS - Scheduler Progress Notifications (#2023)
  DBP: Implement sign-out flow for DBP (#2009)
  Switch CI to Xcode 15.1 (#2022)
  fix(duckplayer): bump CSS for duckplayer nav loop fix (#1982)
  Bump Submodules/privacy-reference-tests from `a3acc21` to `6b7ad1e` (#2006)
  Bump version to 1.69.0 (99)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  DBP: Respect foreign constraints when deleting all user's data (#2014)
  Don't force reload tab when restoring state (#2016)
  Bump version to 1.69.0 (98)
  Update embedded files
  Update NetP launch agent logic to include build number (#2015)
  Always use 'sandbox' Application Support directory for Favicons Fetcher (#2013)
  Allow calculations in the address bar (#2012)
  Fix clickable area for buttons in 'Sync with Another Device' view (#2011)
  Fix for empty autofill state displayed on top of existing password items (#1998)
  Update error messages (#1999)
  ...

# Conflicts:
#	DuckDuckGo.xcodeproj/project.pbxproj
#	DuckDuckGo/Statistics/PixelEvent.swift
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bunn Bunn Bunn approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ayoy @Bunn @tomasstrba